Fix: Remove incorrect 'Closed' label for merged PRs in reviewed section #177

manhuu14 · 2025-07-05T06:58:27Z

Description

Fixes the bug where merged PRs were showing "Closed" label in the reviewed PRs section.

Changes Made

Modified the PR state logic to handle unknown merge status more appropriately
Changed default behavior from showing "Closed" to showing "Open" when merge status is unknown
This specifically affects the reviewed PRs section where the user doesn't own the PRs

Testing

Merged PRs no longer show incorrect "Closed" label
Actually closed PRs still show "Closed" label correctly
Unknown status PRs default to "Open" instead of "Closed"

Related Issue

Fixes #176

Screenshots

[Add before/after screenshots if possible]

Summary by Sourcery

Fix PR state labeling in the reviewed section to distinguish merged, closed, and unknown statuses accurately

Bug Fixes:

Correct merged PRs to display a merged label instead of "Closed" in the reviewed section
Prevent unknown merge statuses from defaulting to "Closed" by showing them as "Open"

Enhancements:

Refine PR state logic to explicitly handle merged, closed, and unknown statuses

- Fixed logic that showed 'Closed' label for merged PRs when merge status was unknown - Now defaults to 'Open' label for unknown status instead of 'Closed' - Addresses issue #176 where merged reviewed PRs incorrectly displayed 'Closed' label Fixes #176

sourcery-ai · 2025-07-05T06:58:32Z

Reviewer's Guide

This PR refactors the logic for labeling “closed” PR items in the reviewed section by explicitly fetching and evaluating merge status, rendering distinct labels for merged, closed, or defaulting unknown states to an open label.

Class diagram for PR status labeling logic in reviewed section

classDiagram
    class PRItem {
        +string state
        +string repository_url
        +int number
        +string html_url
        +string title
    }
    class PRStatusLabeler {
        +labelPR(PRItem item, mergedStatusResults, githubToken, useMergedStatus, fallbackToSimple)
    }
    PRStatusLabeler ..> PRItem : uses

    class MergedStatusResults {
        +map<string, bool> mergedStatus
    }
    PRStatusLabeler ..> MergedStatusResults : queries

Flow diagram for PR status label selection in reviewed section

flowchart TD
    A[PR state is 'closed'] --> B{Fetch merged status}
    B -->|merged === true| C[Show 'Merged' label]
    B -->|merged === false| D[Show 'Closed' label]
    B -->|merged is null/undefined| E[Show 'Open' label]

File-Level Changes

Change	Details	Files
Rewrite handling for closed PR items to distinguish merged, closed, and unknown states	Retrieve merge status when githubToken or mergedStatusResults is available Render merged_button if merged===true Render closed_button if merged===false Render open_button and neutral label for merged===null/undefined	`src/scripts/scrumHelper.js`

Assessment against linked issues

Issue	Objective	Addressed
#176	Remove the 'Closed' label from merged PRs in the reviewed PRs section.	✅
#176	Ensure that actually closed PRs still show the 'Closed' label correctly.	✅
#176	For PRs with unknown status, default to showing 'Open' instead of 'Closed' in the reviewed PRs section.	✅

Possibly linked issues

[Bug] Closed label is visible for merged reviewed PRs #176: PR fixes the bug where merged PRs in reviewed section incorrectly showed 'Closed' label by changing logic for unknown status.
[Bug] Closed label is visible for merged reviewed PRs #176: The PR fixes the bug where merged PRs were labeled 'Closed' by implementing distinct states for merged, closed, and unknown PRs, directly addressing the issue.

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @manhuu14 - I've reviewed your changes and they look great!

Prompt for AI Agents

Please address the comments from this code review:
## Individual Comments

### Comment 1
<location> `src/scripts/scrumHelper.js:977` </location>
<code_context>
                     li += `</li>`;
-                } else if (item.state === 'closed') {
-                    let merged = null;
-                    if ((githubToken || (useMergedStatus && !fallbackToSimple)) && mergedStatusResults) {
-                        let repoParts = repository_url.split('/');
-                        let owner = repoParts[repoParts.length - 2];
-                        let repo = repoParts[repoParts.length - 1];
-                        merged = mergedStatusResults[`${owner}/${repo}#${number}`];
-                    }
-                    if (merged === true) {
</code_context>

<issue_to_address>
Potential edge case if repository_url is malformed.

Accessing array indices after splitting repository_url may cause errors if the URL is malformed. Add validation or guards to handle unexpected input.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

src/scripts/scrumHelper.js

sourcery-ai

Hey @manhuu14 - I've reviewed your changes and they look great!

Prompt for AI Agents

Please address the comments from this code review:
## Individual Comments

### Comment 1
<location> `src/scripts/scrumHelper.js:985` </location>
<code_context>
+    } else if (merged === false) {
+        // PR is confirmed closed but not merged
+        li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a> ${pr_closed_button}</li>`;
+    } else {
+        // merged === null/undefined - status unknown
+        // For reviewed PRs section, we should be more conservative about showing "closed"
+        // since these aren't the user's PRs. Default to showing no specific state label
+        // or a neutral label instead of assuming "closed"
+        li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a> ${pr_open_button}</li>`;
+        
+        // Alternative approach: Don't show any state label for unknown status
</code_context>

<issue_to_address>
Defaulting to 'open' label for unknown merged status may be misleading.

Displaying 'open' when merged status is unknown may mislead users, as the PR is closed but its merged state is unclear. Consider a neutral or absent label to better reflect this ambiguity.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
    } else {
        // merged === null/undefined - status unknown
        // For reviewed PRs section, we should be more conservative about showing "closed"
        // since these aren't the user's PRs. Default to showing no specific state label
        // or a neutral label instead of assuming "closed"
        li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a> ${pr_open_button}</li>`;

        // Alternative approach: Don't show any state label for unknown status
        // li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a></li>`;
    }
=======
    } else {
        // merged === null/undefined - status unknown
        // Do not show any state label for unknown status to avoid misleading users
        li = `<li><i>(${project})</i> - Made PR (#${number}) - <a href='${html_url}'>${title}</a></li>`;
    }
>>>>>>> REPLACE

</suggested_fix>

### Comment 2
<location> `src/scripts/scrumHelper.js:977` </location>
<code_context>
                     li += `</li>`;
-                } else if (item.state === 'closed') {
-                    let merged = null;
-                    if ((githubToken || (useMergedStatus && !fallbackToSimple)) && mergedStatusResults) {
-                        let repoParts = repository_url.split('/');
-                        let owner = repoParts[repoParts.length - 2];
-                        let repo = repoParts[repoParts.length - 1];
-                        merged = mergedStatusResults[`${owner}/${repo}#${number}`];
-                    }
-                    if (merged === true) {
</code_context>

<issue_to_address>
Potential edge case if repository_url is malformed.

Accessing repository_url by index may fail if its structure is inconsistent. Please add validation to ensure repository_url is well-formed before splitting and accessing its parts.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

src/scripts/scrumHelper.js

sourcery-ai · 2025-07-05T07:15:55Z

src/scripts/scrumHelper.js

+} else if (item.state === 'closed') {
+    let merged = null;
+    if ((githubToken || (useMergedStatus && !fallbackToSimple)) && mergedStatusResults) {
+        let repoParts = repository_url.split('/');
+        let owner = repoParts[repoParts.length - 2];


issue: Potential edge case if repository_url is malformed.

Accessing repository_url by index may fail if its structure is inconsistent. Please add validation to ensure repository_url is well-formed before splitting and accessing its parts.

can you resend the suggestion to merge this pr beacuse me bymistakely clicked resolve option

Sure! I'm generating a new review now.

Hey @manhuu14, I've posted a new review for you!

} else if (item.state === 'closed') {
const merged = getMergedStatus(item, repository_url, item.number, githubToken, useMergedStatus, fallbackToSimple, mergedStatusResults);

if (merged === true) { // Confirmed merged li = `<li>(${project}) - Made PR (#${number}) - <a href='${html_url}'>${title}</a> ${pr_merged_button}</li>`; } else if (merged === false) { // Closed but not merged li = `<li>(${project}) - Made PR (#${number}) - <a href='${html_url}'>${title}</a> ${pr_closed_button}</li>`; } else { // Unknown merge status: show no state label to avoid misleading users li = `<li>(${project}) - Made PR (#${number}) - <a href='${html_url}'>${title}</a></li>`; }

}
and function getMergedStatus(item, repository_url, prNumber, githubToken, useMergedStatus, fallbackToSimple, mergedStatusResults) {
let merged = null;

if ((githubToken || (useMergedStatus && !fallbackToSimple)) && mergedStatusResults) { try { const parsedUrl = new URL(repository_url); const pathParts = parsedUrl.pathname.split('/').filter(Boolean); // remove empty strings if (pathParts.length >= 2) { const owner = pathParts[pathParts.length - 2]; const repo = pathParts[pathParts.length - 1]; const prKey = `${owner}/${repo}#${prNumber}`; if (mergedStatusResults.hasOwnProperty(prKey)) { merged = mergedStatusResults[prKey]; // true, false, or undefined } } else { console.warn('[scrum_helper] URL path too short to parse owner/repo:', parsedUrl.pathname); } } catch (err) { console.warn('[scrum_helper] Invalid repository_url:', repository_url, err); } } return merged;

}
add this and try

Based on your latest changes and the review discussion, your PR is ready to be merged! You have addressed the feedback by making the merged status logic more robust and handling edge cases for malformed URLs. If your tests are passing and everything works as expected, you can go ahead and merge the PR.

If you need a final approval from a maintainer, just request it�otherwise, you're good to go!

Let me know if you need help with the merge process or anything else.

i made another pr and there is no isuue ai saing all good how to request maintainer

To request a review from a maintainer on your new PR, follow these steps:

Go to your pull request page on GitHub.

On the right sidebar, look for the "Reviewers" section.

Click the gear icon or the "Request" button next to "Reviewers."

Search for the maintainer's GitHub username (or select from the list) and click their name to add them as a reviewer.

They will be notified and can review your PR.

If you don't see the "Reviewers" section, you may not have permission to request reviewers directly. In that case, you can leave a comment on the PR mentioning the maintainer (e.g., @maintainer-username) and politely ask them to review your PR.

Let me know if you need more help!

if i leave this one automatically merge or not

No, your pull request will not merge automatically if you leave it (unless your repository has an auto-merge feature enabled, which is uncommon). Normally, after your PR is approved and all checks pass, someone (either you or a maintainer) needs to manually click the "Merge" button on GitHub to complete the process.

If you want your PR to be merged, make sure to:

Get all required approvals

Pass all required checks (like CI tests)

Then, either merge it yourself (if you have permission) or ask a maintainer to merge it for you

Let me know if you need help with the merge process or have any other questions!

sourcery-ai

Hey @manhuu14 - I've reviewed your changes - here's some feedback:

Consider extracting the merged/closed/unknown labeling logic into a helper function to reduce nesting and improve readability.
Instead of manually splitting repository_url, use a URL parser or regex utility to safely extract owner and repo and avoid edge-case URL formats.
Double-check that defaulting unknown merge status to pr_open_button matches the intended UX for reviewed PRs, or consider a neutral/no‐label display instead.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- Consider extracting the merged/closed/unknown labeling logic into a helper function to reduce nesting and improve readability.
- Instead of manually splitting `repository_url`, use a URL parser or regex utility to safely extract `owner` and `repo` and avoid edge-case URL formats.
- Double-check that defaulting unknown merge status to `pr_open_button` matches the intended UX for reviewed PRs, or consider a neutral/no‐label display instead.

## Individual Comments

### Comment 1
<location> `src/scripts/scrumHelper.js:977` </location>
<code_context>
                     li += `</li>`;
-                } else if (item.state === 'closed') {
-                    let merged = null;
-                    if ((githubToken || (useMergedStatus && !fallbackToSimple)) && mergedStatusResults) {
-                        let repoParts = repository_url.split('/');
-                        let owner = repoParts[repoParts.length - 2];
</code_context>

<issue_to_address>
The merged status lookup logic could be extracted for clarity.

Consider moving the merged status determination into a helper function to simplify the code and enhance maintainability.

Suggested implementation:

```javascript
                    let merged = getMergedStatus(item, repository_url, githubToken, useMergedStatus, fallbackToSimple, mergedStatusResults);

```

```javascript

```

```javascript
                        });
                    }
                    li += `</li>`;

function getMergedStatus(item, repository_url, githubToken, useMergedStatus, fallbackToSimple, mergedStatusResults) {
    let merged = null;
    if ((githubToken || (useMergedStatus && !fallbackToSimple)) && mergedStatusResults) {
        let repoParts = repository_url.split('/');
        let owner = repoParts[repoParts.length - 2];
        let repo = repoParts[repoParts.length - 1];
        let prKey = `${owner}/${repo}#${item.number}`;
        if (mergedStatusResults.hasOwnProperty(prKey)) {
            merged = mergedStatusResults[prKey];
        }
    }
    return merged;
}

```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

src/scripts/scrumHelper.js

vedansh-5 · 2025-07-05T13:07:15Z

@manhuu14 Please add screenshots of the fix

manhuu14 · 2025-07-06T03:49:22Z

@vedansh-5 sorry for that my Laptop was not able to perform that much actions, some os problem.

sourcery-ai bot reviewed Jul 5, 2025

View reviewed changes

src/scripts/scrumHelper.js Show resolved Hide resolved

sourcery-ai bot reviewed Jul 5, 2025

View reviewed changes

src/scripts/scrumHelper.js Show resolved Hide resolved

vedansh-5 mentioned this pull request Jul 6, 2025

Fix: Remove incorrect 'Closed' label for merged PRs in reviewed section #178

Closed

manhuu14 closed this Jul 9, 2025

manhuu14 deleted the fix-closed-label-merged-prs branch July 9, 2025 08:37

Fix: Remove incorrect 'Closed' label for merged PRs in reviewed section #177

Fix: Remove incorrect 'Closed' label for merged PRs in reviewed section #177

Uh oh!

Conversation

manhuu14 commented Jul 5, 2025 • edited by sourcery-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes Made

Testing

Related Issue

Screenshots

Summary by Sourcery

Uh oh!

sourcery-ai bot commented Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide

Class diagram for PR status labeling logic in reviewed section

Flow diagram for PR status label selection in reviewed section

File-Level Changes

Assessment against linked issues

Possibly linked issues

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sourcery-ai bot Jul 5, 2025

Choose a reason for hiding this comment

Uh oh!

manhuu14 Jul 5, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot Jul 5, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot Jul 5, 2025

Choose a reason for hiding this comment

Uh oh!

manhuu14 Jul 5, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot Jul 5, 2025

Choose a reason for hiding this comment

Uh oh!

manhuu14 Jul 5, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot Jul 5, 2025

Choose a reason for hiding this comment

Uh oh!

manhuu14 Jul 5, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot Jul 5, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vedansh-5 commented Jul 5, 2025

Uh oh!

manhuu14 commented Jul 6, 2025

Uh oh!

Uh oh!

manhuu14 commented Jul 5, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Jul 5, 2025 •

edited

Loading